Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing localized names on boundaries #1022

Merged
merged 10 commits into from
Sep 9, 2016
Merged

Conversation

iandees
Copy link
Member

@iandees iandees commented Sep 1, 2016

  • Update tests
  • Update data migrations
  • Update docs

Connected to #1016.

@@ -60,6 +60,7 @@ WHERE

SELECT
tags->'name' AS name,
'openstreetmap.org' AS source,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

Please also specify Natural Earth as a source at the top of the file in the macro:

{% macro ne_boundary_cols() %}
     gid AS __id__,
     'naturalearthdata.com' AS source,
     {% filter geometry %}the_geom{% endfilter %} AS __geometry__,
     mz_calculate_json_boundaries(t.*) AS mz_properties
 {% endmacro %}

@iandees
Copy link
Member Author

iandees commented Sep 6, 2016

The tests appear to be failing because the right- and left-handedness of the New York/New Jersey boundary is flipped again. Is that arbitrary?

@zerebubuth
Copy link
Member

The code is intended to work from a stable orientation but it looks like the post-processor is sensitive to feature order. I suspect that the left/right are getting flipped because the linestring direction is being flipped - so I think it would still display correctly, which might explain why we haven't noticed this problem until now.

Should we enforce a given ordering, perhaps by sorting on fid or tags['name'] before that loop in the transform?

@iandees
Copy link
Member Author

iandees commented Sep 7, 2016

What the heck? Now the tags aren't there at all?

@zerebubuth
Copy link
Member

It looks like there was an error around the boundaries test, and it logged this:

./scripts/../integration-test.py:148: UnicodeWarning: Unicode unequal comparison failed to convert both arguments to Unicode - interpreting them as being unequal
  elif v != exp_v:

So perhaps they are there, and the unicode unequal comparison means it's picking up the maritime boundary as the "best match", which wouldn't have the same left/right.

@iandees
Copy link
Member Author

iandees commented Sep 7, 2016

That should have been fixed in 39797ff (part of this branch) – I wonder what broke. I'll keep looking locally.

@iandees
Copy link
Member Author

iandees commented Sep 7, 2016

This still behaves differently on my local instance, but appears to pass here now.

@iandees
Copy link
Member Author

iandees commented Sep 7, 2016

(By "differently" I mean that the New York and New Jersey left/right are flipped.)

@zerebubuth
Copy link
Member

Are you running the CI process locally (i.e: getting data from Overpass), or testing against a metro (or other) extract? Would it be possible to grab the GeoJSON for the tile on your local machine and compare it to dev and see if it's just the tags which are flipped or the direction of the linestring too?

@iandees
Copy link
Member Author

iandees commented Sep 7, 2016

After discussing in person, I'm going to add a sort as described in #1022 (comment) so that we get consistent results.

@iandees
Copy link
Member Author

iandees commented Sep 7, 2016

Whew, finally got it working.

selfie-0

@nvkelso
Copy link
Member

nvkelso commented Sep 9, 2016

@zerebubuth Please review :)

@zerebubuth
Copy link
Member

Looks good to me 👍

@iandees iandees merged commit 32dc59d into master Sep 9, 2016
@iandees iandees removed the in review label Sep 9, 2016
@iandees iandees deleted the 1016-missing-localized-names branch September 9, 2016 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants